Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add revive-node version #3

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Add revive-node version #3

wants to merge 11 commits into from

Conversation

smiasojed
Copy link

@smiasojed smiasojed commented Nov 20, 2024

Test with hardcoded Revive - DO NOT MERGE

"lint": "npx eslint 'src/**/*.{cjs,js,ts}' && npx prettier --check '**/*.{ts,json,yml,md}'",
"lint:fix": "npx prettier --write '**/*.{ts,json,yml,md}'",
"test": "npm run build && node ./dist/index.test.js"
"test": "npm run build && node ./dist/index.test.js",
"download:revive": "echo 'TODO: Download Revive release from github'"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about downloading the resolc.{js,wasm} and worker.js files from the Github Revive release instead of creating another one npm package with Revive compiler.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should end up having both? I.e. a JS release and an NPM package

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for the browser JS release, the point is that I don't see the reason to have two separate NPM packages — one for the output of the revive compilation and the other for the NPM js-revive package.

@smiasojed smiasojed marked this pull request as ready for review November 22, 2024 20:38
package.json Outdated
@@ -32,6 +34,7 @@
},
"dependencies": {
"@types/node": "^22.9.0",
"deasync": "^0.1.30",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just temporary solution, will be removed soon

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@smiasojed smiasojed marked this pull request as draft November 22, 2024 20:41
src/worker.js Outdated
@@ -0,0 +1,59 @@
'use strict';

var worker_threads = require('worker_threads');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using CommonJS here, but I compile to an ES module. Therefore, I needed to use Rollup to convert it to CommonJS.

Another consideration is whether we should embed worker.js directly into resolc.js to ship a single JavaScript file. However, that approach seems a bit hacky: Web Workers without a separate JavaScript file.
Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just release both, worker.js and resolc.js

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed worker.js

"lint": "npx eslint 'src/**/*.{cjs,js,ts}' && npx prettier --check '**/*.{ts,json,yml,md}'",
"lint:fix": "npx prettier --write '**/*.{ts,json,yml,md}'",
"test": "npm run build && node ./dist/index.test.js"
"test": "npm run build && node ./dist/index.test.js",
"download:revive": "echo 'TODO: Download Revive release from github'"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should end up having both? I.e. a JS release and an NPM package

@pgherveou
Copy link
Collaborator

@smiasojed merged #4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants